Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session replay improvements #9

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Conversation

MikeShi42
Copy link
Contributor

  • Add explicit email filter
  • Add empty replay message info
  • Filter out empty replay sessions from the replay page as it's usually confusing (we may want to revisit using the replay table instead of the event stream table for filtering though - open to comments)
image

@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2023

🦋 Changeset detected

Latest commit: 3b4715f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MikeShi42 MikeShi42 changed the title Replay improvements: Session replay improvements Sep 19, 2023
@@ -1090,9 +1089,36 @@ export const getSessions = async ({
],
);

const sessionsWithRecordingsQuery = SqlString.format(
`WITH sessions AS (${sessionsWithSearchQuery}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be nice to merge queries for readablility (also make query parameterization migration a bit easier). but no big deal, I can clean it up later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case this query execution is conditional (as this is a slower path), unless you were thinking of having the original query CTE duplicated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was thinking to have CTE inline basically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'ma merge this as-is right now, and we can probably revisit that later - likely we'll need to tune this due to perf anyways.

Copy link
Contributor

@wrn14897 wrn14897 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@MikeShi42 MikeShi42 merged commit bd37a5e into main Sep 19, 2023
2 checks passed
@MikeShi42 MikeShi42 deleted the mikeshi/session-replay-improvements branch September 19, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants